-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix: Remove multicast group from cache when group is uninstalled #4176
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4176 +/- ##
==========================================
+ Coverage 58.35% 58.60% +0.25%
==========================================
Files 374 374
Lines 53893 54085 +192
==========================================
+ Hits 31447 31695 +248
+ Misses 20069 20006 -63
- Partials 2377 2384 +7
*This pull request uses carry forward flags. Click here to find out more.
|
@@ -58,7 +58,7 @@ function generate_mocks { | |||
"pkg/controller/querier ControllerQuerier testing" | |||
"pkg/flowaggregator/exporter Interface testing" | |||
"pkg/ipfix IPFIXExportingProcess,IPFIXRegistry,IPFIXCollectingProcess,IPFIXAggregationProcess testing" | |||
"pkg/ovs/openflow Bridge,Table,Flow,Action,CTAction,FlowBuilder testing" | |||
"pkg/ovs/openflow Bridge,Table,Flow,Action,CTAction,FlowBuilder,Group,BucketBuilder testing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any new auto-generated code by this udpate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with this change we could use gomock on Group and BucketBuilder in unit test.
fba5b89
to
9204af8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
9204af8
to
b04253d
Compare
/test-all |
/test-e2e |
/test-multicast-e2e |
/test-multicast-e2e |
/test-multicluster-e2e |
An issue exists in the code when calling UninstallGroups in multicast feature, that the group entry in the cache is not removed. This is because the existing code only remove group in featureService cache. To resolve the issue, rename the existing API UninstallGroup as UninstallServiceGroup, and add a new API UninstallMulticastGroup to delete multicast related groups. The new API also removes the group entry in the cache. Signed-off-by: wenyingd <[email protected]>
b04253d
to
d216562
Compare
/test-all |
1 similar comment
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
…ntrea-io#4176) An issue exists in the code when calling UninstallGroups in multicast feature, that the group entry in the cache is not removed. This is because the existing code only remove group in featureService cache. To resolve the issue, rename the existing API UninstallGroup as UninstallServiceGroup, and add a new API UninstallMulticastGroup to delete multicast related groups. The new API also removes the group entry in the cache. Signed-off-by: wenyingd <[email protected]>
An issue exists in the code when calling UninstallGroups in multicast
feature, that the group entry in the cache is not removed. This is
because the existing code only remove group in featureService cache.
To resolve the issue, rename the existing API UninstallGroup as
UninstallServiceGroup, and add a new API UninstallMulticastGroup to
delete multicast related groups. The new API also removes the group
entry in the cache.
Fixes #4175
Signed-off-by: wenyingd [email protected]